Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assimilate status code from non-error objects #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Assimilate status code from non-error objects #37

wants to merge 1 commit into from

Conversation

jasonkarns
Copy link

The createError factory is already fairly versatile. It accepts Error objects and assimilates their name/status/stack. It accepts numbers and strings (taking the appropriate status). And it also accepts objects, assimilating their properties.

However, when given an object that contains status or statusCode, it does not assimilate that value.

My particular use case is for errors that are received from various api libraries, particularly REST APIs and Swagger clients. These objects do not inherit from Error, but the quite frequently have a status property.

If this change is accepted, then the following errorware would be quite versatile indeed.

app.use((err, req, res, next) => next(createError(err)));

It would properly support the following middleware scenarios:

  • next(404) -> would become http 404
  • next("Some Error") -> would become an http 500
  • next(new Error("foo")) -> would become http 500, with useful stack trace
  • next({status: 403, foo: "some error object thrown by an api client or library"}) -> would become http 403, assimilating other props

The first three use-cases are already supported by http-errors. The last scenario would make createErrors consistent across all types of input.

I currently accomplish it via: createError(err.status || err.statusCode, err) which handles all the use cases describe above. (Though it relies on the now deprecated usage of number as second argument.)

I can add tests if this change would be considered welcome.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to wait a while to get feedback from everyone, but I would at least expect it to pass the current tests. You didn't describe (to me) a really breaking change, but yet the tests are broken, so I'm not sure if your implementation or your description represents what behavior you are expecting, if you can clarify.

@jasonkarns
Copy link
Author

It could be a breaking change, depending...

createError(400, {status: 500}) is currently expected to result in a 400. Specifically, the status property is explicitly ignored when copying properties from the input (line 103). So either:

  • the status param should take precedence over the status property from props param
    or
  • introduce a breaking change, wherein status from props param is respected

I've gone ahead and changed the code to adhere to the former option, because that makes more sense as desired behavior to me. However, this differs from current behavior for Error objects. (When an Error instance is provided to the factory function, its status takes precedence over any status param. This seems like a bug to me, and could be trivially fixed in this PR if so desired.)

The latter option would be a breaking change but would be consistent with current behavior for Error objects. (Though, again, I almost consider it a bug.)

_err = new Error()
_err.status = 404
err = createError(500, _err) // err.status == 404

@dougwilson
Copy link
Contributor

It isn't a bug; you can see from the tests it is the current intended behavior.

@jasonkarns
Copy link
Author

None of the existing tests ensure the behavior as described. (I "fixed" the behavior without touching the tests, and they're still green.)

@dougwilson
Copy link
Contributor

Ah, I see. I looked at the test I was thinking and you're right.

@jasonkarns
Copy link
Author

Opened #38 to determine if the Error behavior is intended or a bug.

@dougwilson
Copy link
Contributor

I'm fine either way, but need to get the Koa folks engaged here, as this module is one of thier top level APIs.

@jasonkarns
Copy link
Author

need to get the Koa folks engaged here

for sure. And I'll add some new tests soon.

All the existing tests are green except the node 0.6 job fails when attempting to install/compile node via nvm.

@dougwilson
Copy link
Contributor

Need to set dist to precise as of a month ago. I can push that fix to master and you can rebase your branch. To get it 😄

@jasonkarns
Copy link
Author

jasonkarns commented Sep 8, 2017

rebased. tests green! (with added tests)

@dougwilson dougwilson dismissed their stale review July 18, 2018 16:06

tests added

@dougwilson
Copy link
Contributor

Hi @jasonkarns I'm honestly I just let this drop off the face of GitHub. I'm looking to release a new version of the module and so just trying to clean up issues / PR when I noticed it.

I went ahead and squash + rebased your branch so it was up-to-date.

Reading back through the comments trying to refresh my memory, I guess it looks like part of the reason this has been sitting is because it has a semver-major change included. That's actually fine because I am looking to bump the major version of this module very soon to drop Node.js 0.6 support anyway, so easy enough to go ahead and include this PR with that 👍

So I guess the only questions / comments I have are

  1. With the rebase I noticed all the tests are failing. Looks like it's just a simple logic issue with createError(null) throwing out in this new code since it assumes that object type can have properties (null is an object, but throws on property access). Easy enough I can fix that for you 👍
  2. Since this is landing in a semver-major anyway as soon as I hear back, just wanted to circle around on this is the end implementation logic you want, is that right?

@dougwilson dougwilson removed the ideas label Jul 18, 2018
@dougwilson dougwilson self-assigned this Jul 18, 2018
@dougwilson dougwilson added the pr label Jul 18, 2018
@dougwilson dougwilson changed the title Proposal: Assimilate statuscode from non-error objects Assimilate status code from non-error objects Jul 18, 2018
@jasonkarns
Copy link
Author

Yep still looks right to me.

  • status from an Error instance takes precedence over all other inputs
  • status param takes next precedence
  • otherwise, status or statusCode from props object (assuming you cover the props == null case)

Thanks!

@thewilkybarkid

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants